Bundle script instead of curl #851

arthuralee · 2015-04-15T00:23:44Z

This is an initial stab at creating a node-based bundling script so that you don't have to manually run curl. My idea was to have it as a command under react-native. It allows command line flags for dev and minify. It still requires the server to be up, but I hope to detect that and maybe automatically run the server in the future.

Wondering if this is a good idea and if so, the right approach.

drkibitz · 2015-04-16T18:21:16Z

Suggestion:

http.get(options, function(res) {
    var stream = fs.createWriteStream(OUT_PATH);
    res.pipe(stream);
});

arthuralee · 2015-04-17T04:52:10Z

Thanks @drkibitz, took your suggestion.

frantic · 2015-04-17T05:53:13Z

local-cli/cli.js

@@ -14,7 +15,8 @@ function printUsage() {
    '',
    'Commands:',
    '  start: starts the webserver',
-    '  install: installs npm react components'
+    '  install: installs npm react components',
+    '  build: builds the javascript bundle for offline use'


did you mean bundle?

Good catch! I was actually debating build vs bundle

Let's keep it bundle, we are currently thinking about new build command that would compile ObjC code so you don't even have to run Xcode

frantic · 2015-04-17T05:59:32Z

Thanks, that's really handy! We use something similar internally, but it's tied to our infra and build tools.

Actually react packager exposes API to create offline bundle without running the server, see buildPackageFromUrl. Would be awesome to use that instead of asking to run server manually. The only thing to keep in mind is ulimit - the OSX default 256 is way too low. @amasad - any thoughts on using graceful-fs?

And please update the comments in SampleApp's AppDelegate :)

amasad · 2015-04-17T15:39:48Z

@frantic I'll have to update multiple projects we depend on, and hopefully it doesn't cause any performance degradation (it will for people with lower ulimits). But I'm fine with that.

arthuralee · 2015-04-18T02:12:26Z

I've updated the patch. It now uses ReactPackager and does not need the server running.

Some todos:

Take care of ulimit problem (Is this only a problem on some computers? Mine seems to be unlimited and I don't remember ever changing it)
Add a command to choose platform (blacklist)
Add a command line argument for output path

frantic · 2015-04-19T02:48:49Z

@arthuralee - great job! I don't think you should be solving ulimit issue in this PR, that's a more general problem that might be relevant to other tools as well. We should probably either switch to graceful-fs, wrap react-native-cli in a shell script that calls ulimit or use something like node-posix to call setrlimit directly.

I'll take a closer look and merge this in soon.

frantic · 2015-04-19T02:53:08Z

local-cli/bundle.js

+      fs.writeFile(OUT_PATH, bundle.getSource({
+        inlineSourceMap: false,
+        minify: flags.minify
+      }), function() {


Please handle the error here and show some helpful error message.

arthuralee · 2015-04-19T05:43:29Z

Thanks @frantic, sounds good. I've added error handling to fs.writeFile.

Maybe we should address the ulimit problem in a separate issue. I can open one if there isn't one yet.

frantic · 2015-04-20T23:39:38Z

local-cli/bundle.js

+
+  console.log('Building package...');
+  ReactPackager.buildPackageFromUrl(options, url)
+    .then(function(bundle) {


nit: use done to make sure if ReactPackager.buildPackageFromUrl throws the error is reported.

Added bundle script Pipe http response straight to file Used ReactPackager directly, minor fixes Added error handling to fs.writeFile Changed .then to .done

arthuralee · 2015-04-21T00:02:16Z

Makes sense, updated. I also updated the website docs.

Bundle script instead of curl

frantic · 2015-04-21T22:17:15Z

Awesome thanks! If you are willing to make this even better, here is a suggestion - add a step to end-to-end tests that calls react-native bundle and makes sure iOS/main.jsbundle got updated (by checking file size or looking for a substring in the file)

arthuralee · 2015-04-21T22:24:05Z

Great to see this merged! Will look into e2e tests. :)

amasad · 2015-04-23T20:58:45Z

👯

tom76kimo · 2015-04-25T08:07:23Z

Nice work! When will this be released? Just noticed the live Docs has updated but the cli tool hasn't. May introduce confusions. 😄

arthuralee · 2015-04-25T23:36:07Z

Good point. It's already been merged so its available on master, but not on @latest/@0.4.0 which is what people will get over npm install. Maybe at some point we'll need to versioning in the documentation...

romanonthego · 2015-04-29T12:01:54Z

does bundle command included correctly in 0.4.1?
i've recieve error

ZapCourier [master●] % react-native bundle
Building package...

/Users/romanonthego/Code/ZapCourier/node_modules/react-native/node_modules/bluebird/js/main/promise.js:626
            throw e;
                  ^
Error: EMFILE, open '/Users/romanonthego/Code/ZapCourier/node_modules/react-native/node_modules/react-tools/src/browser/ui/dom/ViewportMetrics.js'

every time.
npm start + curl still works fine.

on same notice - why react 0.4.0 and 0.4.1 does not marked as latest release? is it considerate unstable or is it just a github problem (npm @latest installs 0.4.1)?

arthuralee · 2015-04-29T15:23:49Z

Thanks @romanonthego. I will look into this today. There have been a lot of new changes since this has been merged, so it might have broken something. Looks like I should've gone into e2e tests earlier! In the meantime, feel free to create an open issue for this.

As far as I know, @latest installs the latest tagged version, which is currently 0.4.1.

arthuralee · 2015-04-29T15:50:43Z

@romanonthego I could not repro the error. I tested both an empty project and an existing project. To further verify this, the file in question (/node_modules/react-native/node_modules/react-tools/src/browser/ui/dom/ViewportMetrics.js) seems to be bundled correctly and in main.jsbundle. Could you perhaps produce a minimal example of the bug?

frantic · 2015-04-30T06:17:10Z

@romanonthego - the problem is caused by too many files and low open files limit per process. Run ulimit -n 2560 in your terminal before running react-native bundle.

@arthuralee, it's hard to repro because different environments might have different ulimits, also some users will have more js files in node_modules and will hit the bug, wheres vanilla react-native can fit in standard 256 limit.

romanonthego · 2015-04-30T06:27:23Z

I've just cleaned npm cache and reinstall all global packages (react-native and react-native-cli including).
Bug is gone, all works like a charm.

@frantic i've used this fix and it didn't help at the moment (I've encountered this bug before, although it was in form Error: EMFILE, too many open files and not Error: EMFILE, open #{path}).

See facebook#20154

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 15, 2015

arthuralee mentioned this pull request Apr 15, 2015

jsCodeLocation choice #821

Closed

arthuralee force-pushed the master branch from 6634b66 to 598635b Compare April 17, 2015 04:49

frantic reviewed Apr 17, 2015
View reviewed changes

vjeux added the Needs Review label Apr 17, 2015

arthuralee force-pushed the master branch from 598635b to ef209c1 Compare April 18, 2015 02:06

frantic reviewed Apr 19, 2015
View reviewed changes

frantic added GH Review: needs-revision and removed GH Review: review-needed labels Apr 19, 2015

arthuralee force-pushed the master branch from ef209c1 to becddde Compare April 19, 2015 05:40

reactjs-bot added GH Review: review-needed and removed GH Review: needs-revision labels Apr 19, 2015

frantic reviewed Apr 20, 2015
View reviewed changes

arthuralee added 2 commits April 20, 2015 20:01

Added bundle command using ReactPackager

c13646f

Added bundle script Pipe http response straight to file Used ReactPackager directly, minor fixes Added error handling to fs.writeFile Changed .then to .done

Updated docs to reflect bundle script

3adcc03

arthuralee force-pushed the master branch from becddde to 3adcc03 Compare April 21, 2015 00:01

frantic added a commit that referenced this pull request Apr 21, 2015

Merge pull request #851 from arthuralee/master

4792707

Bundle script instead of curl

frantic merged commit 4792707 into facebook:master Apr 21, 2015

brentvatne mentioned this pull request Apr 26, 2015

react-native bundle command not recognized #1027

Closed

ayushjainrksh referenced this pull request in MLH-Fellowship/react-native Jul 2, 2020

ScrollView zoom is not supported on Android (#851)

d448bec

See facebook#20154

acoates-ms pushed a commit to acoates-ms/react-native that referenced this pull request Oct 26, 2021

Remove deprecated/unused context param (facebook#851)

86af69f

snyk-bot mentioned this pull request Jan 17, 2022

[Snyk] Fix for 16 vulnerabilities jrogatis/react-native#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundle script instead of curl #851

Bundle script instead of curl #851

arthuralee commented Apr 15, 2015

drkibitz commented Apr 16, 2015

arthuralee commented Apr 17, 2015

frantic Apr 17, 2015

arthuralee Apr 17, 2015

frantic Apr 17, 2015

frantic commented Apr 17, 2015

amasad commented Apr 17, 2015

arthuralee commented Apr 18, 2015

frantic commented Apr 19, 2015

frantic Apr 19, 2015

arthuralee commented Apr 19, 2015

frantic Apr 20, 2015

arthuralee commented Apr 21, 2015

frantic commented Apr 21, 2015

arthuralee commented Apr 21, 2015

amasad commented Apr 23, 2015

tom76kimo commented Apr 25, 2015

arthuralee commented Apr 25, 2015

romanonthego commented Apr 29, 2015

arthuralee commented Apr 29, 2015

arthuralee commented Apr 29, 2015

frantic commented Apr 30, 2015

romanonthego commented Apr 30, 2015

Bundle script instead of curl #851

Bundle script instead of curl #851

Conversation

arthuralee commented Apr 15, 2015

drkibitz commented Apr 16, 2015

arthuralee commented Apr 17, 2015

frantic Apr 17, 2015

Choose a reason for hiding this comment

arthuralee Apr 17, 2015

Choose a reason for hiding this comment

frantic Apr 17, 2015

Choose a reason for hiding this comment

frantic commented Apr 17, 2015

amasad commented Apr 17, 2015

arthuralee commented Apr 18, 2015

frantic commented Apr 19, 2015

frantic Apr 19, 2015

Choose a reason for hiding this comment

arthuralee commented Apr 19, 2015

frantic Apr 20, 2015

Choose a reason for hiding this comment

arthuralee commented Apr 21, 2015

frantic commented Apr 21, 2015

arthuralee commented Apr 21, 2015

amasad commented Apr 23, 2015

tom76kimo commented Apr 25, 2015

arthuralee commented Apr 25, 2015

romanonthego commented Apr 29, 2015

arthuralee commented Apr 29, 2015

arthuralee commented Apr 29, 2015

frantic commented Apr 30, 2015

romanonthego commented Apr 30, 2015